Conversation
toumorokoshi
left a comment
There was a problem hiding this comment.
Nice! This looks really thorough, and from a code standpoint, all looks really good.
I think there's a couple areas from a design standpoint that's worth talking through. I've added comments above but:
- can we share more with ext-wsgi? duplicate code can become hard to maintain, especially as we begin to spilt codebases out of this one.
- the choice for the 4 spans per request will be fairly noisy. How does this look in practice? Is this how all tracing ASGI implementations work today?
| # TODO: Update once | ||
| # https://github.com/open-telemetry/opentelemetry-specification/issues/270 | ||
| # is resolved | ||
| return scope.get("path", "/") |
There was a problem hiding this comment.
looking at the conversation in the opentelemetry-specification, we should use the HTTP method rather than the specialized path, since it has very high cardinality: https://github.com/open-telemetry/opentelemetry-specification/pull/416/files.
But this is a bug with the existing wsgi implementation as well.
There was a problem hiding this comment.
Here's a tracking ticket for the wsgi implementation, but it'd be good to fix it in ASGI now: #409
There was a problem hiding this comment.
Acknowledged, I'll change it to utilize the method_name.
Do you feel like I should make a similar change in the WSGI implementation to keep them equivalent, or would you rather have that change seperately?
There was a problem hiding this comment.
It appears the recent update to the specification includes
Instrumentation MUST NOT default to using URI
path as span name, but MAY provide hooks to allow custom logic to override the
default span name.
Might I suggest adding an optional callback to this middleware to allow clients to specify a override the default span name?
There was a problem hiding this comment.
I'll add an optional callback to this, and pass it through.
I will make a similar change to the WSGI implementation in another PR, just to keep them syncronized.
| ] | ||
|
|
||
|
|
||
| def http_status_to_canonical_code(code: int, allow_redirect: bool = True): |
There was a problem hiding this comment.
It feels like we should share this with the wsgi implementation somehow. Would it be bad if we just imported methods from the wsgi implementation, and made those more general?
There was a problem hiding this comment.
I was considering this myself, and had a version which imported this function from the WSGI extension instead of keeping it here, as it is untouched and thus identical.
My argument against this, and why I changed it back, is as this would introduce a dependency between the two implementation, and I did not feel comfortable with that with regards to packaging and shipping the package.
Thus, if I was to share code between the modules, I'd argue to refractor out common functionality into a utilities module, but I'm not sure where to put that.
As for sharing the get_header_from_x functionality, it would simply require a function transforming WSGI headers to ASGI headers or vice versa.
- We can make this change, if that's something we want.
There was a problem hiding this comment.
Yeah, I see less of a rationale for get_headers_form_x since it requires a conversion of wsgi headers (although I could see us abstracting that out with a constructor).
I feel like MAYBE this is something we can put in the API. But would like maybe a second opinion on that.
There was a problem hiding this comment.
Alright, I'll leave the get_headers_from_x alone.
Do you think I should refactor out the http_status_to_canonical_code to a seperate module? - say opentelemetry-ext-httputil similar to the opentelemetry-ext-testutil? - or would we rather just deal with code-duplication or interdependencies between ext-asgi and ext-wsgi?
There was a problem hiding this comment.
My two cents: I'd vote for a separate module, or even add it to the SDK (new ext-sdk?) The OTEL spec defines this mapping explicitly, so may as well provide it in the SDK right?
|
Could use this pattern to exclude python versions prior to 3.7 in coverage. I suspect you'll also want to update the |
I'm actually in the process of doing this right now. |
Signed-off-by: Emil Madsen <sovende@gmail.com>
b1da910 to
e0184fb
Compare
…TP METHOD Signed-off-by: Emil Madsen <sovende@gmail.com>
3e34d36 to
710586d
Compare
|
@Skeen @majorgreys is picking up finishing this PR in #716. If you would like to come back and finish this up yourself, please ping myself or @majorgreys. |
|
Hi @toumorokoshi, @majorgreys. I've been wanting to get back to this, but I haven't found the time. I'm sorry. By all means feel free to take the work and finish it up for me. |
This change implements an ASGI counterpart to the WSGI extension.
It has been tested with django channels on a dummy project here: https://github.com/Skeen/django_opentelemetry_example
WSGI Jaeger trace:

ASGI Jaeger trace:

ASGI websocket Jaeger trace:
